SLES-2913: prevent named pipe failures from eating the whole thread pool#231
Open
apiarian-datadog wants to merge 2 commits into
Open
SLES-2913: prevent named pipe failures from eating the whole thread pool#231apiarian-datadog wants to merge 2 commits into
apiarian-datadog wants to merge 2 commits into
Conversation
Author
|
This will also hopefully take care of #219 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2fc929af02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Let's hope this fixes #219 🤞 |
Author
|
validated with https://github.com/DataDog/serverless-examples/pull/205 . works in isolation and in combination. |
| { | ||
| // WriteAsync overload with a CancellationToken instance seems to not work. | ||
| return _namedPipe.WriteAsync(buffer, 0, length).Wait(_timeout); | ||
| if (_namedPipe.WriteAsync(buffer, 0, length).Wait(_timeout)) |
Member
There was a problem hiding this comment.
I know this code was there before your PR, but using Wait()on a asyncTask` is considered bad practice and can easily lead to deadlocks.
Not a blocker for your PR, but maybe we should leave a TODO comment for someone to fix later?
lucaspimentel
approved these changes
Jul 13, 2026
Copilot stopped reviewing on behalf of
lucaspimentel due to an error
July 13, 2026 16:22
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds safeguards to prevent runaway blocking/overlapping behavior in named-pipe sending and telemetry flushing, with new tests to validate the intended throttling and non-overlap behavior.
Changes:
- Add a cooldown gate to
NamedPipeTransportto fail fast after send/connect/write failures. - Change telemetry timer to one-shot re-arming to prevent overlapping flushes and thread pool growth.
- Add unit tests covering named pipe cooldown behavior and telemetry flush overlap/dispose scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| tests/StatsdClient.Tests/Transport/NamedPipeTransportTests.cs | Adds timing-based tests validating cooldown avoids repeated blocking on connect/write failures. |
| tests/StatsdClient.Tests/TelemetryTests.cs | Adds tests asserting flush callbacks don’t overlap and dispose doesn’t deadlock during an in-flight flush. |
| src/StatsdClient/Transport/NamedPipeTransport.cs | Implements failure cooldown gating and resets cooldown on successful send. |
| src/StatsdClient/Telemetry.cs | Switches to one-shot timer re-arm pattern with locking + disposed flag to prevent overlapping flushes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+83
to
+109
| [Test] | ||
| public void ConnectionCooldownAvoidsRepeatedBlocking() | ||
| { | ||
| var connectTimeout = TimeSpan.FromSeconds(1); | ||
| var cooldown = TimeSpan.FromSeconds(10); | ||
|
|
||
| // No server listens on this pipe, so Connect blocks for the full timeout and fails. | ||
| using (var transport = new NamedPipeTransport("cooldownPipeNameTest", connectTimeout, cooldown)) | ||
| { | ||
| var stopwatch = Stopwatch.StartNew(); | ||
| Assert.False(transport.Send(_buffToSend, _buffToSend.Length)); | ||
| var firstSendDuration = stopwatch.Elapsed; | ||
|
|
||
| stopwatch.Restart(); | ||
| for (int i = 0; i < 5; i++) | ||
| { | ||
| Assert.False(transport.Send(_buffToSend, _buffToSend.Length)); | ||
| } | ||
|
|
||
| var cooldownSendsDuration = stopwatch.Elapsed; | ||
|
|
||
| // The first send pays the connect timeout; subsequent sends within the | ||
| // cooldown fail fast instead of each blocking on Connect again. | ||
| Assert.That(firstSendDuration, Is.GreaterThan(TimeSpan.FromMilliseconds(500))); | ||
| Assert.That(cooldownSendsDuration, Is.LessThan(TimeSpan.FromMilliseconds(500))); | ||
| } | ||
| } |
Comment on lines
+111
to
+135
| [Test] | ||
| public void WriteTimeoutTriggersCooldown() | ||
| { | ||
| var timeout = TimeSpan.FromSeconds(1); | ||
| var cooldown = TimeSpan.FromSeconds(10); | ||
| var pipeName = "writeCooldownPipeNameTest"; | ||
| var releaseServer = new ManualResetEventSlim(false); | ||
|
|
||
| // The server connects but never reads, so its buffer fills and the client's | ||
| // write stalls until it times out. | ||
| var serverTask = Task.Run(() => | ||
| { | ||
| using (var serverStream = new NamedPipeServerStream( | ||
| pipeName, | ||
| PipeDirection.In, | ||
| 1, | ||
| PipeTransmissionMode.Byte, | ||
| PipeOptions.Asynchronous, | ||
| _serverBufferSize, | ||
| 0)) | ||
| { | ||
| serverStream.WaitForConnection(); | ||
| releaseServer.Wait(); | ||
| } | ||
| }); |
Comment on lines
+159
to
+161
| releaseServer.Set(); | ||
| serverTask.Wait(); | ||
| } |
Comment on lines
+135
to
+137
| [Test] | ||
| public void FlushesDoNotOverlap() | ||
| { |
Comment on lines
+153
to
+155
| // Make a flush slow relative to the interval so overlapping callbacks would | ||
| // be observable if the timer were still periodic. | ||
| Thread.Sleep(20); |
| using (new Telemetry( | ||
| new MetricSerializer(new SerializerHelper(null, null), string.Empty), | ||
| "1.0.0.0", | ||
| TimeSpan.FromMilliseconds(50), |
Comment on lines
+176
to
+177
| Assert.AreEqual(1, maxConcurrentSends); | ||
| } |
Comment on lines
+189
to
208
| private void OnTimerFlush() | ||
| { | ||
| try | ||
| { | ||
| Flush(); | ||
| } | ||
| finally | ||
| { | ||
| // Re-arm the one-shot timer only after this flush finishes, so flushes cannot overlap. | ||
| // Dispose sets _disposed and disposes the timer under the same lock, so while _disposed | ||
| // is false the timer is still alive. | ||
| lock (_timerLock) | ||
| { | ||
| if (!_disposed) | ||
| { | ||
| _optionalTimer?.Change(_flushInterval, Timeout.InfiniteTimeSpan); | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
+79
to
+88
| if (_namedPipe.WriteAsync(buffer, 0, length).Wait(_timeout)) | ||
| { | ||
| _sendFailureTimer.Reset(); | ||
| return true; | ||
| } | ||
|
|
||
| // The write timed out. The pipe still reports connected, so cool down to | ||
| // avoid re-blocking for the full timeout on every subsequent send. | ||
| _sendFailureTimer.Restart(); | ||
| return false; |
duncanpharvey
approved these changes
Jul 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dogstatsdin Azure App Services with the Windows Extension sometimes fail to correctly get a named pipe. This can happen when the app is hot-reloaded and the previous incarnation ofdogstatsdhasn't released the named pipe when the newdogstatsdis starting up. This causes the named pipe writes from the dogstatsd client to fail (permanently, until the app is restarted again).These failures cause a steady growth of threads as the telemetry timer spawns one every 10 seconds to try to write client telemetry, and the 2 second timeout for each of the 11 stats that the telemetry wants to send compounds to 22 seconds of work. Forever.
This change turns the telemetry timer into one that reschedules itself after every round, instead of spawning one every 10 seconds. It also adds a cooldown for sending telemetry, so that if there's a failure to send, we back off for a bit before trying to send any more.
This is one of a few different patches to address this issue. We also have DataDog/dd-trace-dotnet#8874 for the app service extension process manager and DataDog/datadog-agent#53328 for dogstatsd.